Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

Review: PR #44 — fix(factory): suppress concurrent duplicate dispatches

Summary

The PR wraps FactoryLoop.dispatch() with an in-memory #dispatchInFlight map keyed by issueKey(decision.issue) (key:uuid:path). A concurrent call for the same key returns the already-running promise and bumps a dispatchDuplicateSuppressed counter; the original body is moved verbatim into #dispatchUnlocked. The wrapper itself is well-formed (sets the map entry before the first await yields, and the finally only deletes its own promise).

Blocking finding — re-entrant self-deadlock (PR-introduced regression)

The new lock deadlocks on the Slack triage-escalation replay path, which re-enters dispatch() for the same issue key while the outer dispatch is still unresolved:

• dispatch() stores promise P in #dispatchInFlight[key], then calls #dispatchUnlocked (factory.ts:1299).
• #dispatchUnlocked takes the escalation branch and awaits #escalateTriageToSlack (factory.ts:1340) → #postAndWatchSlackEscalationThread → #watchSlackThread, which replays an existing human answer inline within that await chain.
• The replay reaches #startOrQueueSlackClarifiedDecision, which calls this.dispatch(decision, …) (factory.ts:3812). dispatchAfterSlackClarification/issueWithSlackClarification preserve the issue ref, so issueKey is identical to the outer call.
• The inner dispatch() finds P in the map and returns it (factory.ts:1293-1297), then awaits P. P cannot resolve until #dispatchUnlocked finishes — which is blocked on this very replay. Deadlock.

Reproduced deterministically: src/orchestrator/factory.test.ts:6846 ("replays an existing human Slack answer when a triage escalation watcher starts") times out at 5000ms with the PR applied. The test is pre-existing (not in this PR's diff) and passed before the lock was added, so this is a regression that fails CI (npm test).

I did not patch this. The fix sits squarely in dispatch / in-flight lock logic, which is safety-critical and requires human judgment (the correct shape — e.g., having the re-entrant clarified-dispatch path call #dispatchUnlocked directly, or scoping the lock so the replay doesn't await its own parent promise — changes runtime behavior and must not be auto-edited). A human-authored patch is needed; the regression test at factory.test.ts:6846 already guards the correct behavior, so no test change should be required once the code is fixed.

Verification performed (CI-equivalent): npm ci → npm run build (tsc + tsc-alias) passes clean; npm test (vitest) = 545 passed, 1 failed (the deadlock above).

Addressed comments

• No bot or human review comments were present in the harness inputs (.workforce/context.json carries no review data; no comment files in .workforce/). Nothing to account for.

Advisory Notes

• The committed package-lock.json is stale (records version 0.1.5 while package.json is 0.1.11); npm ci regenerates it with churn. Out of scope for this PR — I restored the lockfile so no unrelated change is pushed. Worth a separate lockfile-refresh PR.

Working tree

Clean — no edits left behind (the npm ci lockfile change was reverted with git restore; the untracked memory/ directory is pre-existing and unrelated).

Not printing READY: a required CI check (npm test) is failing due to the deadlock regression, and the needed fix is safety-critical dispatch/in-flight code that must be authored by a human rather than auto-applied.

Addressed PR feedback and CI failure.

Changes:

• Included effective dry-run/live mode in the dispatch lock key so dry-runs cannot suppress real dispatches.
• Used stable Linear issue key identity for the lock instead of the path/uuid-sensitive batch key.
• Split escalation vs dispatch lock phases so Slack clarification replay can re-enter dispatch without self-deadlocking.
• Added regression coverage for same Linear key through different mirror paths.

Validation:

• npm run build
• npm test
• npx vitest run src/orchestrator/factory.test.ts -t "does not double-dispatch the same issue from concurrent live events"
• npx vitest run src/orchestrator/factory.test.ts -t "dedupes concurrent dispatches for the same Linear key across mirror paths"
• npx vitest run src/orchestrator/factory.test.ts -t "replays an existing human Slack answer when a triage escalation watcher starts"

GitHub CI package check is now passing on commit c010b20.

Review of PR #44 — fix/live-dispatch-inflight-lock

Summary of change

The PR adds an in-process single-flight guard around FactoryLoop.dispatch. The public dispatch method now computes a per-issue key (<issue.key>:<dry-run|live>:<dispatch|escalation>), memoizes the in-flight Promise<DispatchResult> in a new #dispatchInFlight map, returns the existing promise for concurrent duplicates (incrementing dispatchDuplicateSuppressed), and delegates the real work to a renamed private #dispatchUnlocked. A test verifies two concurrent dispatches for the same Linear key arriving via mirror paths (canonical + alias) collapse to one dispatch.

Correctness & safety assessment

• The single-flight pattern is implemented correctly: the promise is stored before await, duplicates return the cached promise, and the finally cleanup uses an identity check (this.#dispatchInFlight.get(key) === dispatched) so a newer entry under the same key is never clobbered. (factory.ts:1291)
• #dispatchUnlocked is a pure rename of the previous dispatch body — no behavioral lines were altered inside it.
• The new in-memory #dispatchInFlight map is distinct from the existing persistent #clearDispatchInFlight/#state.releaseInFlight mechanism (factory.ts:1858); they operate at different layers and do not conflict.
• The dedupe key includes phase and dryRun, so a dry-run and a live dispatch, or a dispatch vs. an escalation for the same key, are not incorrectly collapsed. This preserves existing semantics.
• No fail-closed→fail-open changes, no guard-default changes, no truthiness→presence swaps.

I did not auto-edit any of this code. It is dispatch/in-flight logic — safety-critical territory — but it is the declared purpose of this PR (an intentional, human-authored concurrency fix), and I found no defect requiring a fix. There was nothing mechanical (lint/format/typo/import-order) to clean up in the diff.

Verification (full CI command run end-to-end)

• npm ci — ok
• npm run build (tsc -p tsconfig.build.json && tsc-alias) — typecheck clean
• npm test (vitest run) — 32 files, 547 tests, all passing (new test included)
• npm pack --dry-run — ok

Addressed comments

• No bot or reviewer comments were present in the harness materials (.workforce/context.json contains only PR metadata; no review-comment file was provided). Nothing to address.

Advisory Notes

• None.

The change is small, well-scoped to its stated purpose, correct, and the full CI build/test/pack passes locally. No required checks appear pending and there are no findings requiring code changes; the remaining merge decision is a human judgment call.